-
Notifications
You must be signed in to change notification settings - Fork 43
feat: refine work with membership info and other meta information #2341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
size-limit report 📦
|
197ba46
to
d0da8f8
Compare
a824fff
to
08af18e
Compare
@danisharora099 can you create an issue capturing this problem(s)? and can you, please, make PR name more informative? |
} | ||
|
||
// Initialize members and subscriptions | ||
this.fetchMembers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to improve it even further and:
- use similar approach to https://github.com/waku-org/js-rln/blob/f6d5deb8cb51602b2b544de5de5b88de96e11aef/src/contract/rln_contract.ts#L56 for both contract implementations;
- inject an instance of base contract into specific contract and not use
extend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I preferred inheritance in this specific example to avoid writing this delegation boilerplate: e8aa95f :D
Generally, composition >>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into some membership cache management issues when I tried to compose it instead of inherit -- seems like some tests need to be refactored
Creating a separate issue: #2392 -- will do it as a separate PR
@weboko can you please highlight which comments are blocking comments, and which are nits that aren't as important? |
a7f52e4
to
6d86b6f
Compare
@@ -7,7 +7,7 @@ export type Password = string | Uint8Array; | |||
|
|||
// see reference | |||
// https://github.com/waku-org/nwaku/blob/f05528d4be3d3c876a8b07f9bb7dfaae8aa8ec6e/waku/waku_keystore/protocol_types.nim#L111 | |||
export type MembershipInfo = { | |||
export type KeystoreMembershipInfo = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what is the point of renaming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were two instances of MembershipInfo
:D
@danisharora099 I try to mark nit comment as nit and others we can at least discuss - here is comment that was ignored but raises important architectural change #2341 (comment) |
845bbfc
to
9b71499
Compare
e8aa95f
to
7d63996
Compare
7d63996
to
a03cb71
Compare
Problem / Description
There are a few problems with the current adapter:
idCommitment
is a stringified hexgetMembershipInfo
doesn't return all available membership info fieldsvalidateRateLimit
validates the upper and lower bounds defined in the adapter, instead of the contractSolution
bigint
foridCommitment
instead ofstring
getMembershipInfo
to fetch, and return, ALL available dataNotes
Checklist